-
Notifications
You must be signed in to change notification settings - Fork 27.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lramos15/replace on save #143144
Lramos15/replace on save #143144
Conversation
await group?.replaceEditors([{ editor, replacement: result, options: editorOptions }]); | ||
if (resolvedEditor !== ResolvedStatus.ABORT && isEditorInputWithOptionsAndGroup(resolvedEditor)) { | ||
// dispose the previous result before replacing with the resolved editor | ||
saveResults.pop()?.dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lramos15 I am not so sure about this dispose
call on the editor input. We typically never call dispose
on editor inputs because the lifecycle may be different depending on the kind of input. For example, all file editor inputs are shared across all groups.
Why is this dispose
call needed? It will happen as part of the replaceEditors
call via a close
call I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well normally we replace editor
with result
but here we are replacing editor
with resolvedEditor
meaning the editor input returned from save (result
) isn't being used. So I dispose of it and add the resolved editor as the result instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm yeah, I think this would resolve automatically once save
/ saveAs
no longer returns a typed editor input.
@@ -910,8 +910,16 @@ export class EditorService extends Disposable implements EditorServiceImpl { | |||
if (!result.matches(editor)) { | |||
const targetGroups = editor.hasCapability(EditorInputCapabilities.Untitled) ? this.editorGroupService.groups.map(group => group.id) /* untitled replaces across all groups */ : [groupId]; | |||
for (const targetGroup of targetGroups) { | |||
const resolvedEditor = await this.editorResolverService.resolveEditor({ resource: result.resource, }, targetGroup); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lramos15 I think this solution is a bit of a bandaid: ideally editor.saveAs
and editor.save
would return an untyped editor input and then replaceEditors
should not be called with a typed editor but with an untyped one. This is somewhat a missing adoption of untyped editors in general (I was aware of it but forgot about it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I was just a bit nervous to go with that level refactor given I'm not really sure how sequential vs non sequential save work and all that plays a role. This change while bandaid seemed safe and allows the testing of this flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see in debt week whether a cleaner change is possible.
This PR fixes #81832
Now on save if a replace will be completed we utilize the resolver to change the editor input